TEL-886: Media Port rework - #802
Conversation
11f4b21 to
dd06f29
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Huh, the race in CI is real, and is apparently a problem in media-sdk (yay zerocopy) that previous tests simply did not expose. |
Will fix this in media-sdk. |
6a063e6 to
dce2728
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| // WriteOutboundDTMFTo tells the room where to send DTMF to. | ||
| // Returns the previously-set writer (if one exists). | ||
| WriteOutboundDTMFTo(w msdk.WriteCloser[*livekit.SipDTMF]) msdk.WriteCloser[*livekit.SipDTMF] |
There was a problem hiding this comment.
Any specific reason to use pointers in livekit.SipDTMF? It's a tiny payload, might as well just copy it around.
There was a problem hiding this comment.
I guess the thread safety is a good benefit of copying. Fair enough.
There was a problem hiding this comment.
WriteSample passes lock by value: github.com/livekit/protocol/livekit.SipDTMF contains google.golang.org/protobuf/runtime/protoimpl.MessageState contains sync.Mutex I guess this is one,
There was a problem hiding this comment.
Oh, right, it's a protobuf. Can we swap it for a non-proto struct? I remember we had races on proto structs before.
47b61bd to
5ad9b22
Compare
Fixed in media-sdk#81. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
4c60a45 to
470911b
Compare
|
|
||
| // WriteOutboundDTMFTo tells the room where to send DTMF to. | ||
| // Returns the previously-set writer (if one exists). | ||
| WriteOutboundDTMFTo(w msdk.WriteCloser[*livekit.SipDTMF]) msdk.WriteCloser[*livekit.SipDTMF] |
There was a problem hiding this comment.
Oh, right, it's a protobuf. Can we swap it for a non-proto struct? I remember we had races on proto structs before.
| if s.cli != nil { // Process reinvite for existing outbound calls | ||
| oc := s.cli.getActiveCall(cc.ID()) | ||
| newCSeq := cc.InviteCSeq() | ||
| if oc != nil && oc.cc != nil && oc.cc.InviteCSeq() < newCSeq { |
There was a problem hiding this comment.
This is pre-existing, behavior, but, in a follow-up PR, we should reply with some kind of error if the new sequence number is lower than the existing one (per spec, I think this should be internal server error). Adding a todo.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
But lets land #819 first, as it's safer and easier to validate. |
3 participants
This change is intended to: